verification: fix race same block + misc #11400

niklasad1 · 2020-01-23T11:12:03Z

Ignored propagating the raw bytes in the Error because it requires so many changes instead I changed to Error type for Kind::Create

ordian · 2020-01-23T13:46:07Z

ethcore/src/client/client.rs

-				self.importer.bad_blocks.report(block.bytes, format!("{:?}", err));
-				return Err(EthcoreError::Block(err))
+			Err((EthcoreError::Block(err), input)) => {
+				self.importer.bad_blocks.report(input.map_or(Bytes::new(), |i| i.bytes), err.to_string());


will it report this https://github.com/paritytech/parity-ethereum/blob/e1d06efe60afdd793cb6c9b599fbc9e8c76ac7c1/ethcore/src/client/bad_blocks.rs#L64

if the input is None? Can input be None? It's not supposed to be, right?
Do you think it makes sense to use expect here instead?

Currently, the only condition when input is None is for the introduced ImportError::AlreadyQueued so I think it could be an expect.

However, it might be harder to reason about when maintaining/refactoring the code i.e, if a (BlockError, None) is introduced and then the node would panic just because the raw bytes of the error was missing (but it would still be a bug but not that severe as panic). How about a warning/error message?

Ideally, this would be encoded in type-system (as you suggested) and we wouldn't need to worry about this.

I would just use an expect for now and maybe add some docs to the import method (that for EthcoreError::Block we should return Some(input)) as well as add a TODO here referencing an issue (and file an issue) to encode this in the type system.

But let's see what others think.

Sounds reasonable to me

ordian · 2020-01-24T13:03:05Z

ethcore/verification/src/queue/mod.rs

 			}
 		}

 		match K::create(input, &*self.engine, self.verification.check_seal) {
 			Ok(item) => {
+				if self.processing.write().insert(hash, item.difficulty()).is_some() {
+					return Err((Error::Import(ImportError::AlreadyQueued), None));
+				}


(for the reviewers) the fix is here

tomusdrw

lgtm!

tomusdrw · 2020-01-24T13:44:21Z

ethcore/src/client/client.rs

+			Err((EthcoreError::Block(err), input)) => {
+				self.importer.bad_blocks.report(
+					input
+						.expect("BlockQueue::import enforces that `EthcoreError::Block` always returns Some(input); qed")


Why not match Err((EthcoreError::Block(err), Some(input))) => { instead? Seems it would be more future proof. You could just print an error in case we get (EthcoreError, None) in the next match, so that you can easily detect if the assumption get's broken.
Current proof seems pretty weak to me.

see #11400 (comment)

I agree it's not future proof, but the right fix is to implement #11403. Feel free to disagree though.

the idea hit me too, but let's fix it in #11403?

Well, you know I'm usually against panicking, especially if the assumptions are a bit weak like here: we assume something about internals of generic type and hope it will never change.

I'm more willing to just print an error in case we get None, I don't see how it is better to crash the node than to just not report a bad block. It seems that the risk of introducing the error is not worth the gains you could get from failing fast. The node will be in a perfect state if we don't panic and will continue to operate, the only thing we will miss is a report about bad block.

I fully agree though that #11403 is the right solution, but for now I'd rather avoid panicking.

right, I don't have a strong opinion on that, but my reasoning was that if the panic would happen, it would urge the developers to implement #11403, otherwise it would just sit the backlog for ages

But either way is fine.

niklasad1 · 2020-01-24T13:56:58Z

ethcore/src/client/client.rs

 				gas_used: U256::default(),
 				gas_limit: header.gas_limit(),
 			}
 		})
 	}

-	fn build_last_hashes(&self, parent_hash: &H256) -> Arc<LastHashes> {
+	fn build_last_hashes(&self, parent_hash: H256) -> Arc<LastHashes> {


this was changed was motivated to make it more clear that parent_hash needs ownership instead of cloning it inside the function.

…pstream * master: Add POSDAO transition and malice report queue. (#11245) update master/nightly version: v2.8.0 (#11419) ethcore/res: remove morden testnet (#11392) fix: export hardcoded sync format (#11416) update hardcoded headers: mainnet and ropsten (#11414) AuthorityEngine: Minor cleanups. (#11408) Update POA bootnodes (#11411) Add EtherCore support (#11402) verification: fix race same block + misc (#11400) Update ProgPoW to 0.9.3 (#11407) update classic testnet bootnodes (#11398) dependencies: bump `derive_more v0.99` (#11405) engine error: remove faulty/unused `From` (#11404) Switching to stable-track (#11377) ethcore/res: fix ethereum classic chainspec blake2_f activation block num (#11391) Update copyright notice 2020 (#11386)

* ethcore: fix race in verification * verification: fix some nits * verification: refactor err type `Kind::create` * fix: tests * address grumbles * address grumbles: don't panic

* update classic testnet bootnodes (#11398) * update classic testnet bootnodes * Update kotti.json * Update kotti.json * Update kotti.json * Update mordor.json * verification: fix race same block + misc (#11400) * ethcore: fix race in verification * verification: fix some nits * verification: refactor err type `Kind::create` * fix: tests * address grumbles * address grumbles: don't panic * fix: export hardcoded sync format (#11416) * fix: export hardcoded sync format * address grumbles * make tests compile with rustc_hex 2.0 * fix grumbles: impl LowerHex for encoded Header * goerli: replace foundation bootnode (#11433) * Remove dead bootnodes, add new geth bootnodes (#11441) * update kvdb-rocksdb to 0.4 (#11442) * Avoid long state queries when serving GetNodeData requests (#11444) * Remove dead bootnodes, add new geth bootnodes * More granular locking when fetching state Finish GetDataNode requests early if queries take too long * typo * Use latest kvdb-rocksdb * Cleanup * Update ethcore/sync/src/chain/supplier.rs Co-Authored-By: Andronik Ordian <[email protected]> * Address review grumbles * Fix compilation * Address review grumbles Co-authored-by: Andronik Ordian <[email protected]> * rlp_derive: cleanup (#11446) * rlp_derive: update syn & co * rlp_derive: remove dummy_const * rlp_derive: remove unused attirubutes * rlp-derive: change authors * Cargo.lock: cargo update -p kvdb-rocksdb (#11447) * Cargo.lock: new lockfile format Manual backport of #11448 * gcc to clang (#11453) * gcc to clang test ``` export CC="sccache "$CC export CXX="sccache "$CXX ``` darwin build ``` CC=clang CXX=clang ``` * darwin - > default clang * Bump version and CHANGELOG.md * chore: remove unused dependencies (#11432) * fix: compiler warnings * chore: remove unused dependencies * Update CHANGELOG.md * update Cargo.lock * update CHANGELOG.md * backwards compatible call_type creation_method (#11450) * rlp_derive: update syn & co * rlp_derive: remove dummy_const * rlp_derive: remove unused attirubutes * rlp-derive: change authors * rlp_derive: add rlp(default) attribute * Revert "Revert "[Trace] Distinguish between `create` and `create2` (#11311)" (#11427)" This reverts commit 5d4993b. * trace: backwards compatible call_type and creation_method * trace: add rlp backward compatibility tests * cleanup * i know, i hate backwards compatibility too * address review grumbles * update CHANGELOG.md * just to make sure we don't screw up this again (#11455) * Update CHANGELOG.md Co-authored-by: Talha Cross <[email protected]> Co-authored-by: Niklas Adolfsson <[email protected]> Co-authored-by: Martin Holst Swende <[email protected]> Co-authored-by: David <[email protected]> Co-authored-by: Andronik Ordian <[email protected]> Co-authored-by: Denis S. Soldatov aka General-Beck <[email protected]>

niklasad1 added 3 commits January 17, 2020 17:09

ethcore: fix race in verification

c6868e8

verification: fix some nits

27856bc

verification: refactor err type Kind::create

4d47240

ordian reviewed Jan 23, 2020

View reviewed changes

fix: tests

ef67be9

ordian added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. labels Jan 23, 2020

address grumbles

12c0b77

ordian approved these changes Jan 24, 2020

View reviewed changes

ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Jan 24, 2020

ordian requested a review from tomusdrw January 24, 2020 13:02

ordian reviewed Jan 24, 2020

View reviewed changes

tomusdrw approved these changes Jan 24, 2020

View reviewed changes

niklasad1 commented Jan 24, 2020

View reviewed changes

address grumbles: don't panic

07956cd

ordian merged commit a47a5b1 into master Jan 24, 2020

ordian deleted the na-dup-blocks-ugly branch January 24, 2020 15:51

s3krit mentioned this pull request Feb 5, 2020

V2.7.2 stable #11454

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

verification: fix race same block + misc #11400

verification: fix race same block + misc #11400

niklasad1 commented Jan 23, 2020

ordian Jan 23, 2020

niklasad1 Jan 23, 2020 •

edited

Loading

ordian Jan 23, 2020 •

edited

Loading

niklasad1 Jan 23, 2020

ordian Jan 24, 2020

tomusdrw left a comment

tomusdrw Jan 24, 2020

ordian Jan 24, 2020

niklasad1 Jan 24, 2020 •

edited

Loading

tomusdrw Jan 24, 2020

ordian Jan 24, 2020

niklasad1 Jan 24, 2020 •

edited

Loading

verification: fix race same block + misc #11400

verification: fix race same block + misc #11400

Conversation

niklasad1 commented Jan 23, 2020

ordian Jan 23, 2020

Choose a reason for hiding this comment

niklasad1 Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

ordian Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

niklasad1 Jan 23, 2020

Choose a reason for hiding this comment

ordian Jan 24, 2020

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw Jan 24, 2020

Choose a reason for hiding this comment

ordian Jan 24, 2020

Choose a reason for hiding this comment

niklasad1 Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

tomusdrw Jan 24, 2020

Choose a reason for hiding this comment

ordian Jan 24, 2020

Choose a reason for hiding this comment

niklasad1 Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

niklasad1 Jan 23, 2020 •

edited

Loading

ordian Jan 23, 2020 •

edited

Loading

niklasad1 Jan 24, 2020 •

edited

Loading

niklasad1 Jan 24, 2020 •

edited

Loading